Skip to content

Make the supported examples prove behaviour, not existence (#104) - #446

Merged
jeremymanning merged 1 commit into
mainfrom
fix/trustworthy-supported-examples
Aug 2, 2026
Merged

Make the supported examples prove behaviour, not existence (#104)#446
jeremymanning merged 1 commit into
mainfrom
fix/trustworthy-supported-examples

Conversation

@jeremymanning

Copy link
Copy Markdown
Member

Responds to the review of #445. Covers its item 1 (repair the supported-example
contract) and item 5 (deeper assertions), plus item 3 (subprocess cleanup) and
one defect the stronger assertions surfaced.

The example was demonstrating the opposite of its claim

Confirmed before changing anything:

$ orchestrator run examples/supported/04_conditions.yaml
output/long.txt
output/short.txt        <- the "short" handler ran on the long path

handle_short was never a branch. It was the last link of a dependency chain
(check_size -> handle_long -> handle_short), so the default path fell through
into it. on_false routing worked; the example just did not express exclusive
branches — and EXPECTATIONS asserted both artifacts, pinning it in place.

Rewritten as two exclusive branches converging on a join. The long arm's
on_success: summarise jumps the short arm; the short arm is reached only by
on_false. Measured, both arms:

long (default) short (-i content=hi)
ran check_size, handle_long, summarise handle_short, mark_short, summarise
skipped handle_short, mark_short check_size, handle_long
artifacts long.txt, branch.txt short.txt, branch.txt
taken_branch "long" "short"

taken_branch used to render {{ out_dir }} — it echoed an input and said
nothing about the branch. It now reads a file the branch itself wrote.

What a case asserts now

Exit code; every artifact and its exact contents, with an unexpected file
counting as a failure; the complete outputs document; which steps completed,
were skipped and failed; and the execution levels, so "alpha and beta run in
parallel" is checked rather than asserted in a comment. checkpoints/ is named
as a run side effect and asserted directly instead of silently ignored.

Examples with a branch declare a case per branch, so the arm a default run does
not take still executes in CI.

CLI/API equivalence was nearly vacuous

The comparison dropped every step value and the entire outputs document, on
a comment claiming values embed absolute paths differing by working directory.

Measured across all 5 examples x 3 repetitions, the only differing JSON paths
are started_at, completed_at, duration, execution_id. Every step value
is byte-identical; paths inside them are relative (./output/greeting.txt).
The comment was wrong.

PipelineResult.normalized() already blanked exactly those fields and already
claimed in its docstring to be the CLI/API comparison form, so this extracts
normalize_result_payload(dict) to serve both the object and the CLI's JSON.
One definition — the test can no longer drift into comparing less than it says.

Two runtime defects the stronger assertions surfaced

A timed-out command was abandoned, not killed. asyncio.wait_for cancels
the wait, not the process, and the timeout handler never referred to the child
again:

sleep procs BEFORE: 0
run finished, exit=1
sleep procs IMMEDIATELY AFTER: 2      <- one per attempt, outliving the run

_reap() in a finally also covers the cancellation raised when a step
rather than the command times out. Closing pipes on a live loop is also what
stops the PytestUnraisableExceptionWarning at teardown that #445's review
reported.

Templates dropped a trailing newline — but only for a parameter referencing
another step:

content trailing newline
literal "literal\n" kept
parameter ref "param {{ word }}\n" kept
step ref "step {{ reader.result.size }}\n" lost

That path alone goes through a Jinja Environment left on the default
keep_trailing_newline=False. Of 547 blocking tests, the only two affected
were the ones pinning the truncated byte. Found because the suite now asserts
exact contents; the old suite asserted the file existed.

Evidence the new tests actually work

Every new test was run against the un-fixed code:

Test Without fix With fix
supported-example cases vs the old 04 4 FAIL pass
timed-out command leaves no process FAIL, names the leaked pid pass
template keeps its last newline FAIL pass

The leak test was vacuous in its first draft, and this is how I caught it:
the marker sat in a shell comment, and sh -c "sleep 30 # marker" execs sleep
in place, dropping both the shell and the comment from argv. A renamed copy of
sleep was then SIGKILLed by macOS code signing (rc=137). It now tracks process
ancestry with psutil.

Verification

  • Blocking suite 541 -> 548 passed, 13 skipped, 0 failed
  • Acceptance suite 26 -> 31 tests
  • PytestUnraisableExceptionWarning and ResourceWarning are now errors in
    test_supported_examples.py and test_failure_policy.py
  • compileall clean; working tree clean

On the review's "13 skips vs 12" note: skips depend on which optional extras are
installed, so that number is environment-dependent rather than a discrepancy.

Deliberately not in this PR

  • Non-raising fail-fast (review item 2). Agreed it is the next runtime P0,
    but it changes the execution loop, not the examples.
  • Migration guide for the old {"steps":..., "outputs":...} envelope
    (item 4). docs/CLAUDE.md directs that documentation describe the code as it
    is now and that migration guides not be written unless asked.
  • Issue reconciliation and catalogue expansion (items 5-6). Bookkeeping and
    new examples respectively; neither belongs in a correctness change.

Examples validating is unchanged at 8 of 116 — this PR makes the supported set
trustworthy rather than larger.

🤖 Generated with Claude Code

The acceptance suite asserted that a command exited and that some files
existed. A badly broken runtime satisfies that, and one did: 04_conditions
ran *both* arms of its conditional, and the test asserted both artifacts,
pinning the bug in place.

04_conditions now expresses two genuinely exclusive branches converging on a
join. `handle_short` was never a branch -- it was the last link of a
dependency chain, so the default path fell through into it. The long arm's
`on_success` jumps the short arm; the short arm is reached only by `on_false`.
The declared output `taken_branch` used to render `{{ out_dir }}`, echoing an
input; it now reads a file the branch itself wrote, so it reports the branch.

Each example declares a case per branch, and a case now fixes the exit code,
every artifact *and its exact contents*, the complete outputs document, which
steps completed / were skipped / failed, and the execution levels -- so
"alpha and beta run in parallel" is a checked claim. An unexpected artifact is
a failure. `checkpoints/` is named as a run side effect and asserted directly,
rather than silently ignored.

CLI/API equivalence was nearly vacuous: the comparison dropped every step
value and the whole outputs document, on a comment claiming values embed
absolute paths. Measured across 5 examples x 3 runs, the only fields that ever
differ are the timestamps and the execution id; values are byte-identical and
paths in them are relative. `PipelineResult.normalized()` already blanked
exactly those, so `normalize_result_payload` now serves both the object and
the CLI's JSON and the test cannot drift into comparing less than it claims.

Two runtime defects surfaced from the stronger assertions:

A timed-out command was abandoned rather than killed. `asyncio.wait_for`
cancels the wait, not the process, and the timeout handler never referred to
the child again: 06_failure_policy left two `sleep` processes alive after the
orchestrator had exited. `_reap()` in a `finally` also covers the cancellation
raised when a step rather than the command times out, and closing the pipes on
a live loop is what stops the unraisable-exception warnings at teardown.

Templates dropped the trailing newline of any parameter referencing another
step, because that path alone goes through a Jinja environment left on the
default `keep_trailing_newline=False`. Literals and parameter references kept
theirs. Of 547 blocking tests the only two affected were the ones pinning the
truncated byte.

Every new test was run against the un-fixed code: the example cases fail 4x on
the old 04, the leak test names the surviving pid, the newline test fails on
the lost byte. The first draft of the leak test was vacuous -- its marker sat
in a shell comment, and `sh -c "sleep 30 # marker"` execs sleep in place and
drops it from argv -- so it now tracks process ancestry instead.

Blocking suite 541 -> 548 passed, 13 skipped, 0 failed. Unraisable and
resource warnings are errors in both affected suites.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jeremymanning

Copy link
Copy Markdown
Member Author

CI verification

9/9 green.

Legacy tally against the last post-merge main run (run 30747692522), since
PR-branch runs are not valid baselines:

main #446 delta
failed 475 474 -1
passed 1845 1846 +1
errors 189 189 0
skipped 200 200 0
deselected 640 647 +7

The +7 deselected is exactly the 7 tests this PR adds to the blocking layer
(541 -> 548), which the legacy job runs the complement of. The -1 failed / +1
passed is within the noise floor I revised to +/-3 on #445, so I am not
claiming it as an improvement caused by this change.

One thing I checked rather than assumed

The legacy job's annotations contain two Event loop is closed messages. That
is the same class of defect as the subprocess leak fixed here — an unclosed
transport surfacing at teardown — and this PR adds an await process.wait()
inside a finally, which could plausibly produce exactly that during
interpreter shutdown.

It does not: the count is 2 on main and 2 here, unchanged. Pre-existing
in the quarantined layer, not introduced.

Worth filing separately, though. It is the same failure mode as the one this PR
fixes in TerminalTool, so there is probably at least one more abandoned
subprocess or unclosed transport elsewhere in the legacy surface — the fix here
was scoped to the tool the supported examples actually exercise.

@jeremymanning
jeremymanning merged commit 8433fe2 into main Aug 2, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant